[improvement](inverted index) Push the candidate row bitmap down into phrase queries - #67180
[improvement](inverted index) Push the candidate row bitmap down into phrase queries#67180airborne12 wants to merge 5 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
There was a problem hiding this comment.
Requesting changes for four substantiated issues, represented by three inline anchors (the two config issues share the new definition).
- Candidate pushdown can make the synthetic Roaring iterator the norm source and change BM25 score filtering/top-k results.
- The mutable ratio accepts unsafe values that reach undefined floating-to-integer conversion, including a transient runtime-publication window.
- The one-time gate misses candidate sets that become selective after earlier index conjuncts.
- The 0.1 default cannot activate for the PR's stated 22.4%-selective short-key-only workload, so the claimed default approximately 4.5x improvement does not occur.
Critical checkpoint conclusions:
- Goal and test proof: direct exact-phrase bitmap restriction works in the added unit cases, but score equivalence, production activation, config boundaries, and the advertised default workload are not proved and are contradicted by the findings above.
- Scope and compatibility: the nine-file change is focused. It introduces no storage format, FE/BE protocol, persistence, transaction, data-write, rolling-upgrade, or cloud-specific behavior.
- Concurrency, ownership, lifecycle, and errors: the borrowed candidate bitmap is segment-owned and used synchronously; query objects do not retain it, the DEFER cleanup covers exits, and reader exceptions/status propagation remain sound. No new lock-order, static-lifetime, or memory-accounting issue was found.
- Parallel paths and cache behavior: non-scoring exact/sloppy/ordered match semantics, Boolean/null/virtual candidate-universe algebra, iterator sentinels, and full-result cache hits are sound. Candidate-restricted misses do not escape into a reusable full-segment cache. Multi-term phrase-prefix shares the scoring defect; single-term/phrase-edge/SEARCH-v2/SNII paths do not consume the candidate.
- Configuration, performance, and observability: runtime observation exists, but value safety/publication and default calibration are unsound. The gate also misses later selectivity crossings. Existing inverted-index timers expose aggregate work, but there is no candidate-engagement/cardinality metric.
- Verification: three normal+risk rounds converged with no new valuable finding in the final capped round. No additional user focus was provided. Per the review prompt, no local build or test was run; current CI has formatter/style/license and FE/Cloud checks passing, while BE UT, compile, performance, and automated code-review checks remain pending.
TPC-H: Total hot run time: 16723 ms |
TPC-DS: Total hot run time: 81783 ms |
ClickBench: Total hot run time: 14.47 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
3438d6f to
53b393c
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review result: REQUEST_CHANGES
One new P2 performance issue is reported inline: publishing candidate_rows currently prevents cache warming for legacy fulltext query shapes that never consume the candidate and still compute a full-segment bitmap.
Existing-thread status:
- The scoring path now selects a non-candidate norm source for the regular multi-term phrase path; no distinct new scoring issue was posted.
- The finite/use-site ratio guards and the 0.3 default address the unsafe conversion and stated 22.4% motivating workload.
- The column-predicate-to-expression threshold crossing is now refreshed. Expression-to-expression conjuncts are still evaluated as a batch and intersected only afterward, so a selective earlier MATCH expression cannot shrink the candidate for a later phrase expression; this remains covered by the existing threshold-refresh thread rather than receiving a duplicate inline comment.
Critical checkpoint conclusions:
- Goal and data correctness: Candidate intersection preserves row and score semantics within the published candidate for exact, unordered-sloppy, ordered-sloppy, and multi-term phrase-prefix evaluation. Full cache hits remain safe because SegmentIterator intersects them with
_row_bitmap; no partial data/null/score result was found escaping into a full-result cache. - Scope and performance: The implementation is otherwise focused, but the unconditional cache-insert guard creates a default-on repeated-query regression for non-consuming fulltext query families, so changes are requested.
- Concurrency and lifecycle: Evaluation is synchronous and segment-local. The non-owning candidate pointer is not retained,
_row_bitmapis stable while a query consumes it, and DEFER clears it on success and all Status exits. No lock-order, thread-context, or cross-TU static issue applies. - Configuration and compatibility: The mutable ratio is observed dynamically and guarded before floating-to-integer conversion; non-positive values intentionally disable the feature. There is no FE/BE protocol, storage-format, transaction, persistence, or rolling-upgrade change. SNII/query_v2 paths ignore the new field and remain behaviorally compatible.
- Tests and hygiene: The added unit tests cover candidate restriction, full-cover equivalence, BM25 norm equivalence, column-to-expression threshold crossing, reset, and non-finite direct assignment. Coverage is still missing for non-consuming cache warming and the expression-to-expression boundary. The read-only BE build-hygiene gate passed; per the review environment, no build or test suite was run, and live BE UT/compile/performance checks were pending during review.
- Observability: Existing inverted-index timers and cache statistics are sufficient to diagnose these paths; no transaction/write observability applies.
- User focus: No additional user-provided focus was specified.
Review convergence completed in two rounds. All Round 2 normal and risk-focused agents returned NO_NEW_VALUABLE_FINDINGS, and every candidate is accepted, duplicate-fenced, or dismissed with code evidence.
| cache->insert(cache_key, term_match_bitmap, &cache_handler); | ||
| // A bitmap produced under a candidate restriction is partial and | ||
| // must never be cached as the full-segment result. | ||
| if (context->candidate_rows == nullptr) { |
There was a problem hiding this comment.
[P2] Only skip cache insertion when this query actually consumed the candidate. candidate_rows is published for every fulltext evaluation once the scan is below the ratio, but only multi-term phrase/phrase-prefix joins it. MATCH_ANY, MATCH_ALL/EQUAL, regexp/phrase-edge, and single-term phrase/prefix still compute a full-segment bitmap; after a cold miss this guard nevertheless refuses to cache it, so repeated selective scans re-decode the same full postings forever. Please carry an explicit per-result candidate-consumed/partial-result signal (or otherwise gate on the executed query shape) and add a cold-miss/second-hit test for a non-consuming query alongside the partial phrase-cache test.
TPC-H: Total hot run time: 16976 ms |
TPC-DS: Total hot run time: 82434 ms |
ClickBench: Total hot run time: 14.62 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Automated review result: REQUEST_CHANGES
Two distinct new issues are reported inline:
- P1 correctness: a candidate-restricted phrase TRUE bitmap is combined with a full-segment NULL bitmap and consumed as a full-domain compound result. Nullable
ANDcan short-circuit before a later FALSE child, soNOT (A AND B)can drop a row whose SQL result is TRUE; virtual projections can likewise materialize NULL instead of FALSE. - P2 performance/style: the new release-mode
_norm_sourceinvariant check runs once per scoring match inside the document loop even though the source is immutable afteradd().
Existing-thread status:
- Multi-term phrase-prefix scoring can still select a
UnionTermIteratorwhosenorm()throws; this remains covered by discussion_r3864368344 rather than receiving a duplicate comment. - The finite use-site guard and 0.3 default address the original unsafe-conversion/default-workload portions of discussion_r3864368348.
- Post-column refresh is present, while remaining within-phase refresh opportunities stay fenced by discussion_r3864368353.
- Cache insertion is still suppressed for non-consuming fulltext shapes and remains covered by discussion_r3868272817.
Critical checkpoint conclusions:
- Goal and proof: the change implements selective candidate pushdown for legacy multi-term phrase and phrase-prefix evaluation. Added unit cases prove standalone exact restriction, candidate-lead score equivalence for real term postings, full-cover equivalence, post-column threshold crossing, non-finite use-site rejection, and reset. They do not prove the new nullable compound/domain case, cold-cache behavior, or union phrase-prefix scoring.
- Focus/minimality: the 12-path change is focused on one BE config, the SegmentIterator/context handshake, phrase DISI support, cache eligibility, and tests.
- Concurrency: evaluation is synchronous and segment-local. The candidate bitmap is not mutated while the adapter consumes it; no new thread, lock, atomic, or lock-order surface is introduced.
- Lifecycle/static state:
candidate_rowsborrows SegmentIterator-owned_row_bitmap, is valid for the synchronous reader call, and is cleared by DEFER on normal, downgrade, and error exits. No static-initialization, ownership-cycle, or dangling-pointer issue was found. - Configuration: the mutable ratio is observed at each refresh; the finite and
0 < ratio <= 1use-site guard makes multiplication/conversion safe, and non-positive values intentionally disable the feature. - Compatibility: this is BE-internal and adds no storage-format, persisted-state, public-symbol, FE/BE protocol, cloud, or rolling-upgrade change.
- Parallel paths/conditions: single-term phrase/prefix, other legacy fulltext shapes, SEARCH query-v2, and SNII do not consume the candidate. Leapfrog alignment and exact/sloppy/ordered matching are sound on the active candidate domain. The distinct full-domain three-valued consumer failure is reported inline.
- Tests/results: the new test is discovered by the recursive BE storage-test glob; the read-only build-hygiene gate passed. Per the review prompt, no local build or test suite was run. Live compile, BE/FE/Cloud UT, P0, cloud_p0, vault_p0, performance, formatter, style, license, and secret checks are green; NonConcurrent Regression is failing, while External Regression and the code-review check are pending.
- Observability: existing inverted-index timers, cache statistics, filtered-row counters, scoring instrumentation, and downgrade logging are adequate for this internal optimization.
- Transactions/persistence/writes/transmission: no transaction, EditLog, data-write, crash-atomicity, or FE-to-BE variable-passing path is changed.
- Performance/other: the non-owning Roaring adapter avoids a bitmap copy. The repeated release check is the distinct new hot-path issue; cache and threshold performance concerns remain in their live threads. No further substantiated issue survived the final sweep.
No additional user focus was provided. Review convergence completed in two rounds: both Round 2 normal full-review agents and the dedicated risk agent returned NO_NEW_VALUABLE_FINDINGS, and every candidate is accepted, duplicate-fenced, or dismissed with code evidence.
| // position verification to candidates) but never a matcher's postings, so | ||
| // phrase semantics stay with the real term iterators. | ||
| if (_context->candidate_rows != nullptr) { | ||
| _iterators.emplace_back(std::make_shared<RoaringDocIdIterator>(_context->candidate_rows)); |
There was a problem hiding this comment.
[P1] Do not expose this candidate-restricted bitmap as a full-domain expression result. FunctionMatchBase still pairs it with the full-segment null bitmap, while VCompoundPred stops an AND when the TRUE bitmap is empty. If nullable phrase A has matches only outside the candidate set and a candidate row has A = NULL followed by indexed B = FALSE, the shortcut returns NULL without evaluating B, so NOT (A AND B) drops a row whose SQL result is TRUE. Either make the result domain explicit and make three-valued shortcuts candidate-aware, or avoid this restriction for compound/virtual evaluation; please add a cold-cache nullable compound regression.
There was a problem hiding this comment.
Fixed in f82c48c054d with a RED→GREEN SegmentIterator test using the real VirtualSlotRef -> NOT -> AND expression tree and nullable inverted-index bitmaps.
RED on the previous head reproduced both observable failures for candidate row 0 (NOT(NULL AND FALSE)): the FALSE child was not evaluated because the candidate-restricted TRUE bitmap triggered the AND shortcut, and row 0 was absent from the final TRUE bitmap. The fix resolves a top-level VirtualSlotRef to its underlying expression before the compound-root suppression decision. The identical test is GREEN after the fix, and the full related ASAN filter is 15/15 GREEN.
Normalized triage (arithmetic mean): severity 10/10 (silent wrong result), scenario confidence 10/10 (deterministic RED), production likelihood 3.5/10 (requires the VirtualSlotRef-wrapped nullable compound shape) => 7.83/10, above the 6/10 fix threshold.
| } | ||
| roaring.add(doc); | ||
| int32_t norm = visit_node(*_lead1, Norm {}); | ||
| DORIS_CHECK(_norm_source != nullptr); |
There was a problem hiding this comment.
[P2] Move this invariant check out of the per-document scoring loop. _norm_source is fixed once in add() after _iterators is complete, so a broad scoring phrase repeats the same release-mode DORIS_CHECK for every matched row; root AGENTS.md permits only DCHECK inside performance-sensitive loops. Validate it once before entering the loop when similarity scoring is active, then read the norm here without a release check.
|
run buildall |
TPC-H: Total hot run time: 16912 ms |
TPC-DS: Total hot run time: 81618 ms |
ClickBench: Total hot run time: 14.51 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Review result: no new inline findings on 6c6514b156b7480c0fc75e61384246bb2c45e4db, but this is not an approval. Two substantiated P1 edge cases on the current head remain covered by existing threads, so I did not duplicate them:
- A
VirtualSlotRef-wrapped compound can bypass the root-only candidate suppression and reach the same candidate-domain/three-valued shortcut problem already described in discussion_r3869542660. - Scoring multi-term phrase-prefix can select
UnionTermIteratoras_norm_source, whosenorm()throws; this is the same norm-capable-source problem already described in discussion_r3864368344.
Critical checkpoint conclusions:
- Goal, scope, and proof: the PR adds a segment-candidate handshake so multi-term legacy phrase-family queries evaluate postings/positions only inside a sufficiently small surviving row set. The implementation is focused across config,
IndexQueryContext, the phrase DISI adapter, reader cache policy, SegmentIterator gating, and targeted BE tests. Direct tests cover candidate restriction, full-coverage equivalence, score equivalence, cache consumption policy, cross-reader reset, threshold refresh, compound-root suppression, non-finite config, and teardown. - Data correctness and parallel paths: simple MATCH results are consumed only within the same
_row_bitmapdomain; direct compound roots are evaluated without the candidate; virtual results are materialized only for selected row ids. FullText/StringType/BKD/SNII selection, exact/sloppy/ordered-sloppy/prefix routing, single-term non-consumption, cache hits, and copied contexts were traced. Apart from the two existing P1 cases above, no additional row, null, scoring, or cache corruption path was found. - Concurrency and lifecycle:
IndexQueryContextis segment-local and these reader/query calls are synchronous. The borrowed Roaring bitmap outlives each query object and is mutated only after query execution returns. No new lock ordering, cross-thread publication, circular ownership, or cross-translation-unit static-initialization issue is introduced. - Error and memory handling: reader exceptions are converted to
Status, expression evaluation converts thrown exceptions before the candidate pointer is restored, and the enclosingDEFERclears the handshake on every exit. The adapter is a small query-scoped object; no significant untracked allocation or ownership leak was found. - Configuration:
inverted_index_candidate_pushdown_ratiois mutable and read at each refresh, so runtime changes are observed without restart. The use-site finite/domain guard protects the multiply/cast even during invalid transient publication; non-positive values disable the path and values above one are rejected. The strict threshold and 0.3 default cover the cited 22.4% workload. - Compatibility and persistence: no FE/BE protocol field, function symbol contract, storage format, transaction, EditLog, or data-write path changes. No mixed-version or crash-recovery compatibility work is required for this BE-local optimization.
- Performance and observability: the candidate participates only in the leapfrog approximation and never in positional matchers; full cached bitmaps remain reusable, while actually partial results stay out of cache. Existing inverted-index timers and cache counters cover the affected path. No separate hot-loop, redundant-copy, or observability defect was substantiated beyond the already-filed threshold/cache/check threads.
- Tests and gates: repository header/build hygiene passed all layering, budget, PCH, extern-template, and unity-skip checks. Current-head CI shows COMPILE, BE UT, and P0 Regression passing. The NonConcurrent Regression check is red, but its TeamCity log returned HTTP 401 here, so this review could not establish whether that failure is related. Per the review-only instruction, no local build or test run was attempted.
- User focus: no additional focus was supplied; the full PR was reviewed.
Round 1 converged with both normal full-review agents and the separate risk-focused agent returning NO_NEW_VALUABLE_FINDINGS. All suspicious points were either cleared with code evidence or deduplicated against the existing threads above.
6c6514b to
b9a4f7d
Compare
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
… phrase queries
### What problem does this PR solve?
Problem Summary:
The cost of a phrase-family inverted index query (MATCH_PHRASE /
MATCH_PHRASE_PREFIX multi-term path) is proportional to the whole segment's
postings and positions, regardless of how small the surviving candidate set
already is. On a production log table a 9-minute time window left only 22.4%
of each segment's rows after short-key pruning, yet every phrase conjunct
still walked the full segment: per-query profile showed 2,657 segments,
11,913s of InvertedIndexSearcherSearchExecTime (99.7% of scan cost) and
125 GiB of index reads for a query whose final result was 0 rows. A
microbenchmark against the same code path calibrates the cost model to
~0.9µs per co-occurrence candidate, so evaluation cost tracks candidates,
not results.
Fix: expose the scan's current candidate row bitmap to index queries through
IndexQueryContext (the same SegmentIterator -> reader handshake channel the
count-on-index fast path already uses):
- `IndexQueryContext::candidate_rows`: set by SegmentIterator around the
index-apply phase when the candidate bitmap is smaller than
`num_rows * inverted_index_candidate_pushdown_ratio` (new BE config,
default 0.1, <= 0 disables), reset on every exit path via the existing
DEFER. `_row_bitmap` only shrinks during the applies, so restricting to
its current state stays correct for every later conjunct.
- `RoaringDocIdIterator`: a read-only DISI adapter over the candidate
bitmap. PhraseQuery joins it into the leapfrog intersection (its
doc_freq() is the cardinality, so a small candidate naturally becomes the
lead), while matchers keep only real term iterators -- a classic
two-phase iterator: candidates drive the approximation, terms keep the
position semantics. The single-term path is unchanged (no restriction,
same semantics).
- Query cache: a bitmap produced under a non-null candidate is partial and
is never inserted into the query cache. Cache lookups stay enabled -- a
cached full-segment bitmap intersected later is still correct and
cheaper. While a candidate is engaged this also skips caching for
non-phrase fulltext queries (conservative but correct; the restriction
only engages below the ratio threshold where such caching has little
value).
Expected effect on the reproduced workload: with only short-key pruning the
phrase work drops to the in-window fraction of each segment (~4.5x on the
profiled table, more for smaller windows); combined with selective
companion conjuncts the candidate set collapses further and so does the
phrase cost. Results are unchanged -- verified by an equivalence test where
a full-coverage candidate reproduces the unrestricted result.
### Release note
Inverted index: phrase queries now restrict doc-list intersection and
position verification to the scan's surviving candidate rows when the
candidate set is small (BE config inverted_index_candidate_pushdown_ratio,
default 0.1).
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Same query results; phrase evaluation cost now tracks the candidate set. Fulltext query-cache inserts are skipped while a candidate is engaged. -->
- Does this need documentation?
- [x] No.
- [ ] Yes.
…shed engage gate, safe config domain Addresses the four automated-review findings on the candidate pushdown: 1. Scoring norm source: with a selective candidate as the leapfrog lead, BM25 norms were read from the candidate adapter (constant 1), flattening per-document length normalization. PhraseQuery now pins _norm_source to the first real postings iterator; a red-first test shows two documents of different lengths scored identically under a candidate before the fix and match the unrestricted scores after it. 2. Engage-gate timing: the threshold was sampled only before _apply_inverted_index(), so an entry bitmap above the ratio that indexed predicates then shrank below it never published candidate_rows. The decision is extracted into _refresh_candidate_pushdown() and re-evaluated at the conjunct boundary after column-level index predicates; covered by a threshold-crossing SegmentIterator test (50% entry -> 5% after an indexed predicate must engage). 3. Config domain safety: the ratio is now guarded twice -- a config validator (finite, <= 1.0) plus an std::isfinite/domain check at the use site before the multiply and integer conversion, so a transiently published out-of-domain value can never reach undefined behavior. Covered by a non-finite-ratio test. 4. Default calibration: 0.1 -> 0.3. The motivating workload keeps 22.4% of each segment after short-key pruning, so the previous default never engaged the pushdown for exactly the case it was built for.
… policy, compound three-valued safety Round 2/3 review fixes: - Cache inserts are now gated on actual candidate consumption instead of candidate publication. A reply-direction candidate_rows_consumed flag is set only when PhraseQuery joins the candidate into its leapfrog and is re-armed per search (including the untokenized range path that bypasses match_index_search), so non-consuming queries (MATCH_ANY/ALL, term, regexp, single-term phrase, range) keep caching their full-segment bitmaps while a candidate is engaged. - A candidate-restricted TRUE bitmap paired with the full-segment null bitmap could spuriously trigger VCompoundPred's three-valued AND shortcut (NOT(A AND B) with nullable A: a candidate row's FALSE is mis-typed as NULL and the row is dropped). Compound roots are now evaluated with the candidate suppressed in both the conjunct and virtual-column projection loops; top-level single-predicate consumption keeps the restriction, which is exact within the candidate. - The _norm_source invariant check moved out of the per-document scoring loop; it is validated once before the loop when scoring is active.
… scoring ### What problem does this PR solve? Issue Number: None Related PR: apache#67180 Problem Summary: A VirtualSlotRef-wrapped compound expression bypassed the candidate suppression used to protect SQL three-valued bitmap evaluation. A nullable NOT(A AND B) expression could therefore short-circuit on a partial TRUE bitmap and silently drop a row for which NOT(NULL AND FALSE) is TRUE. Resolve the effective root through VirtualSlotRef before deciding whether to suppress the candidate. Scoring a multi-term phrase-prefix query could also choose a low-frequency UnionTermIterator as the norm source and fail with "UnionTermIterator does not support scoring". Restrict the norm source to the exact TermPositionsIterator, which owns the per-document norm. A no-candidate run reproduces the same failure, confirming that this scoring edge predates candidate pushdown. ### Release note Fix nullable compound inverted-index filtering through virtual columns and multi-term phrase-prefix scoring failures. ### Check List (For Author) - Test: Unit Test - Added RED/GREEN tests for VirtualSlotRef-wrapped nullable three-valued logic. - Added RED/GREEN real-index scoring tests with and without candidate rows. - Ran 15 related ASAN BE unit tests. - Ran ./build.sh --be with BUILD_TYPE=ASAN. - Behavior changed: Yes. Correct query results are preserved and affected scoring queries no longer fail. - Does this need documentation: No
b9a4f7d to
f82c48c
Compare
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Automated review conclusion: request changes for one P2 performance issue.
Reviewed exact head f82c48c054d6cdabee6d949c4be4beeb81848ae1 against base c800f2aa615e9f3ab445da5738c944644faeb986, using the authoritative bundle and existing inline threads as duplicate fences.
- Goal and focus: the patch is focused on restricting legacy multi-term phrase and phrase-prefix evaluation to selective scan candidates, and the local reader/iterator mechanism works. No additional user-provided focus was supplied.
- Correctness and parallel paths: direct candidate-domain evaluation, compound/virtual-root suppression, exact/sloppy/ordered/prefix matching, and exact-term norm sourcing are consistent. Single-term and other non-consuming legacy queries, query-v2 SEARCH, SNII, BKD, and ANN retain their full-domain behavior.
- Cache, lifecycle, concurrency, and errors: only a consuming legacy multi-term phrase marks a result partial, so those results stay out of the query cache while full-domain results remain cacheable. The candidate bitmap is scanner-owned and synchronously consumed, no reader retains it, and reset/error paths are covered by the established DEFER and Status boundaries. No new thread, lock, transaction, or static-lifecycle behavior is introduced.
- Configuration and compatibility: the mutable ratio is finite/range guarded before arithmetic, and its 0/1/exact-threshold behavior is a cost decision. There is no storage-format, protocol, persistence, data-write, or FE/BE compatibility change.
- Performance and observability: one production-reachable gap remains. The final refresh occurs before condition-derived ranges, per-segment delete bitmaps, and external scanner-split ranges are applied, so those late restrictions cannot engage the optimization even when they reduce a segment far below the 0.3 threshold. Existing timers remain sufficient for the path.
- Tests and hygiene: the changed tests cover direct restriction, score equivalence, phrase-prefix norm selection, partial/full cache policy, threshold refresh/reset, non-finite ratios, and nullable compounds, but not threshold crossing caused solely by late condition/delete/split pruning. This was a review-only run, so no build or test suite was executed;
build-support/check-build-hygiene.shpassed on the pinned head.
Review completion: Round 2 converged with all normal and risk-focused reviewers returning NO_NEW_VALUABLE_FINDINGS. Every candidate is accepted, duplicate-fenced, or dismissed with code evidence; the single accepted P2 is attached inline.
| // The column predicates above may have shrunk the bitmap across | ||
| // the engage threshold; refresh the handshake at this conjunct | ||
| // boundary so the expression conjuncts below still benefit. | ||
| _refresh_candidate_pushdown(); |
There was a problem hiding this comment.
[P2] Include late scan restrictions in the phrase candidate. This is the last refresh before phrase evaluation, but condition-derived ranges are intersected only later in this function, and _lazy_init() applies the per-segment delete bitmap and external scanner-split ranges only after the function returns and the DEFER has cleared candidate_rows. A segment above the 0.3 gate here but reduced far below it by zone-map/dictionary pruning, MoW deletes, or a split range therefore still walks full phrase postings. Please form the evaluation candidate from these stable restrictions without making condition-cache results partial (or safely apply them earlier), and add cold-cache coverage for the late-pruning paths.
There was a problem hiding this comment.
Verified and fixed with TDD rather than accepting the scenario from code inspection alone.
RED evidence (ASAN): I added three cold-cache tests covering external scanner row ranges, a per-segment MoW delete bitmap, and condition-derived page ranges. Before the fix all 3/3 failed at the same assertion: the expression observed a null candidate even though each late restriction reduced a 100-row segment to 5 rows (below the 0.3 engage threshold).
The fix moves those stable restrictions onto the same _row_bitmap before inverted-index expression evaluation. The candidate pointer therefore keeps the same lifetime and observes the monotonically shrinking bitmap. This does not create a local/partial candidate object; the final scan domain is unchanged, and external row ranges remain part of the condition-cache digest.
GREEN evidence: the 3 RED tests now pass; 18/18 related ASAN tests pass (candidate pushdown, phrase, and phrase-prefix); clang-format v16/check-format pass; and ./build.sh --be -j 24 succeeds under ASAN.
Normalized decision score: severity 5/10 (performance fallback, no wrong result), production likelihood 7/10 (MoW deletes/splits/page pruning are common, gated by phrase-family queries and threshold crossing), scenario confidence 10/10 (three deterministic reproductions). Arithmetic normalization = (5 + 7 + 10) / 3 = 7.33/10, above the 6/10 repair threshold, so this was worth fixing.
Fixed in 4057b24.
…ates ### What problem does this PR solve? Issue Number: None Related PR: apache#67180 Problem Summary: Candidate pushdown was refreshed before stable scan restrictions were applied. Phrase and phrase-prefix evaluation therefore walked full postings whenever the initial segment bitmap exceeded the engage threshold, even if page pruning, a merge-on-write delete bitmap, or an external scanner split later reduced the scan to a small range. Apply these stable restrictions to the shared row bitmap before inverted-index evaluation so they participate in the candidate gate without changing the final scan domain or condition-cache keying. ### Release note Improve phrase-query candidate pruning for selective scan restrictions. ### Check List (For Author) - Test: Unit Test - Added RED/GREEN cold-cache tests for condition ranges, delete bitmaps, and external row ranges. - Ran 18 related ASAN BE unit tests. - Ran clang-format v16 and check-format. - Behavior changed: Yes. Phrase-family index evaluation can consume stable late-pruning restrictions as candidates. - Does this need documentation: No
|
/review |
|
run buildall |
There was a problem hiding this comment.
Automated review conclusion: complete on head 4057b24. No new distinct valuable findings remain, so this is a summary-only review with zero inline comments. Existing inline threads were treated as hard duplicate fences and their affected current-head paths were rechecked.
Critical checkpoint conclusions:
- Goal and proof: the change makes multi-position phrase and phrase-prefix evaluation intersect the surviving SegmentIterator candidate while preserving real postings for positions and scoring. The changed real-index and SegmentIterator tests exercise restriction/equivalence, scoring norms, cache policy, threshold refresh, late delete/split/condition pruning, compound three-valued logic, config boundaries, and cleanup.
- Scope and clarity: the production change is focused on the candidate handshake, phrase leapfrog adapter, cacheability reply, pruning order, and the required dynamic threshold. The broader pruning reorder was traced through lazy initialization and preserves the final scan domain.
- Concurrency and lifecycle: no new concurrent execution or lock interaction is introduced. Each SegmentIterator owns the context and evaluates synchronously; the non-owning candidate pointer targets the address-stable row bitmap, is read only during a query, and is cleared by the enclosing DEFER on normal, Status-error, and exception exits. No static-initialization or ownership cycle issue was found.
- Configuration: the mutable ratio is read at each refresh, accepts non-positive values as disabled, validates finiteness and the upper bound, and repeats the full finite/domain guard before bounded floating-point arithmetic and conversion.
- Compatibility and parallel paths: there is no persisted format, RPC/protocol, symbol, transaction, write, or rolling-upgrade contract change. Multi-position phrase-family paths consume the candidate; single-term, ANY/ALL/equal, regexp/edge, range, BKD, direct, SEARCH query-v2, and SNII paths remain full-domain or use their established behavior. FullText and StringType cache paths reset and classify results per search.
- Conditions and error handling: threshold, compound-root suppression, VirtualSlotRef unwrapping, and cache-insertion conditions were checked against concrete failure scenarios. Existing catch-and-convert boundaries and Status propagation remain intact.
- Testing and hygiene: no compilation or unit suite was run by this reviewer because the review workflow prohibits builds. The changed tests and their registration were inspected, and the pure-text BE build-hygiene gate passed. Author-reported ASAN/build evidence in the live threads was considered context but was not independently rerun.
- Performance and observability: the candidate participates in the existing document-frequency ordering without entering positional matchers; cache hits remain usable and only actually partial leaf results skip insertion. Existing inverted-index timers and cache statistics remain available. No substantiated new performance or observability defect was found.
- Data correctness, persistence, and writes: the change is read-only and does not alter visibility versions, transactionality, persistence, or write atomicity. Delete bitmap and scanner-range restrictions are applied to the same final scan bitmap before index evaluation.
- Additional review: a suspected enclosing SEARCH DSL cache interaction was independently traced and dismissed because cacheable legacy SEARCH uses query-v2 objects that do not consume candidate_rows, while the SNII reader branch disables the enclosing DSL cache. No accepted or unresolved candidate remains.
Convergence: both complete-coverage reviewers and the separate risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS in Round 1, so the review converged without reaching the three-round cap.
User focus: review_focus.txt specifies no additional focus; the whole PR and its upstream/downstream interactions were reviewed normally.
What problem does this PR solve?
Problem Summary:
The cost of a phrase-family inverted index query (MATCH_PHRASE /
MATCH_PHRASE_PREFIX multi-term path) is proportional to the whole segment's
postings and positions, regardless of how small the surviving candidate set
already is. On a production log table a 9-minute time window left only 22.4%
of each segment's rows after short-key pruning, yet every phrase conjunct
still walked the full segment: per-query profile showed 2,657 segments,
11,913s of InvertedIndexSearcherSearchExecTime (99.7% of scan cost) and
125 GiB of index reads for a query whose final result was 0 rows. A
microbenchmark against the same code path calibrates the cost model to
~0.9µs per co-occurrence candidate, so evaluation cost tracks candidates,
not results.
Fix: expose the scan's current candidate row bitmap to index queries through
IndexQueryContext (the same SegmentIterator -> reader handshake channel the
count-on-index fast path already uses):
IndexQueryContext::candidate_rows: set by SegmentIterator around theindex-apply phase when the candidate bitmap is smaller than
num_rows * inverted_index_candidate_pushdown_ratio(new BE config,default 0.3, domain (0, 1], out-of-domain values are rejected twice: by a
config validator and by an std::isfinite/domain check at the use site), reset on every exit path via the existing
DEFER.
_row_bitmaponly shrinks during the applies, so restricting toits current state stays correct for every later conjunct.
RoaringDocIdIterator: a read-only DISI adapter over the candidatebitmap. PhraseQuery joins it into the leapfrog intersection (its
doc_freq() is the cardinality, so a small candidate naturally becomes the
lead), while matchers keep only real term iterators -- a classic
two-phase iterator: candidates drive the approximation, terms keep the
position semantics. The single-term path is unchanged (no restriction,
same semantics).
is never inserted into the query cache. Cache lookups stay enabled -- a
cached full-segment bitmap intersected later is still correct and
cheaper. While a candidate is engaged this also skips caching for
non-phrase fulltext queries (conservative but correct; the restriction
only engages below the ratio threshold where such caching has little
value).
Measured effect (crossover microbenchmark on a 200k-doc real CLucene
segment, phrase
big red, this PR's code; every point also verifiesrestricted == unrestricted ∧ candidate):
No regression anywhere in the tested domain (the crossover point is beyond
50%), so the 0.3 default is conservative. Gains steepen as the candidate
set shrinks, which is exactly the regime selective companion conjuncts
produce. The production workload (50-way prefix union tail, position
verification at 44% of cost) should sit above this single-term-tail bench
shape. Results are unchanged -- verified by an equivalence test where a
full-coverage candidate reproduces the unrestricted result.
Release note
Inverted index: phrase queries now restrict doc-list intersection and
position verification to the scan's surviving candidate rows when the
candidate set is small (BE config inverted_index_candidate_pushdown_ratio,
default 0.3).
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Review round 1 (automated review, addressed in the second commit):
_norm_sourceis pinned to the first real postingsiterator, so BM25 norms never come from the candidate adapter. Red-first
test: under a candidate, two documents of different lengths scored
identically (2.9729729) before the fix vs. the correct distinct baseline
scores after it.
_refresh_candidate_pushdown()and re-evaluated after column-level indexpredicates, so an entry bitmap above the ratio that indexed predicates
shrink below it still publishes
candidate_rows(threshold-crossing testadded).
std::isfinite/domain guard before the multiply and integer conversion.each segment after short-key pruning; the previous default never engaged
the pushdown for exactly that case. A finer crossover sweep is planned as
a follow-up benchmark.
Review round 2/3 (automated review, addressed in the follow-up commits):
candidate_rows_consumedflag is set only by the candidate-joining phrasepath and reset per search, so non-consuming queries (MATCH_ANY/ALL, term,
regexp, single-term phrase, untokenized range) keep filling the query cache
with their full-segment bitmaps while a candidate is engaged. Red-first
tests: cold-miss/second-hit for MATCH_ANY under a published candidate, plus
a cross-reader stale-flag case (phrase then untokenized range).
full-segment null bitmap could spuriously trigger VCompoundPred's AND
shortcut (NOT(A AND B) with nullable A: FALSE mis-typed as NULL, row
dropped). Compound roots are now evaluated with the candidate suppressed in
both the conjunct and virtual-column loops; top-level single-predicate
consumption keeps the restriction (exact within the candidate). A SQL-level
nullable-compound regression case is planned as a follow-up.
_norm_sourceinvariant check moved out of the per-document scoringloop (release-mode checks stay out of hot loops per AGENTS.md).